-
Notifications
You must be signed in to change notification settings - Fork 986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for StreamingCredentials #3068
Conversation
This enables use cases like credential rotation and token based auth without client disconnect. Especially with Pub/Sub clients will reduce the chnance of missing events.
DEFAULT_REAUTHENTICATE_BEHAVIOUR
- Moved Authentication handler to DefaultEndpoint - updated since 6.6.0
# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java
d483898
to
7185e22
Compare
Defer the re-auth operation in case there is on-going multi Tx in lettuce need to be externally synchronised when used in multithreaded env. Since re-auth happens from different thread we need to make sure it does not happen while there is ongoing transaction.
7185e22
to
b32f84c
Compare
…provider with static one provider Add unit tests for setCredentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks clean from my POV, i got a few questions though.
} | ||
|
||
if (credentialsProvider instanceof StreamingCredentialsProvider) { | ||
if (!isSupportedConnection()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we good with the behavior when its configured with StreamingCredentials but doesnt work that way. It looks like it happens silently and nobody knows about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that one. WIll add a warning to the log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, this is expected. We can have a streaming-enabled credentials provider and disabled the reauthentication
multi = (multi == null ? new MultiOutput<>(codec) : multi); | ||
|
||
if (command instanceof CompleteableCommand) { | ||
((CompleteableCommand<?>) command).onComplete((ignored, e) -> { | ||
if (e != null) { | ||
multi = null; | ||
inTransaction.set(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the cases that we know of it is not a CompleteableCommand
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
I thought it was that all sync commands are not completable commands.
But why are they exempt from these checks I am not sure.
Does MULTI even work with sync commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tishun
Thanks for the review and help with clean up! It looks neat and tidy!
I think there a few changes that lead to change in behavior
Putting some comments and will try to address them with follow up commit.
move credentials() method to RedisCredentialsProvider. Resolve issue with unsafe cast after extending RedisCredentialsProvider with supportsStreaming() method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hey @ggivo - i guess we can close this one, right? |
Description
This PR is preparation step for adding support for Microsoft Entra ID (formerly Azure AD) authentication support
It introduces support for streaming credentials provider allowing connection re-authentication when new credentials are available.
Posible use cases is token based authentication (e.g. expiring tokens) or credential rotation.
PR introduces :
An example of credential provider supporting streaming : MyStreamingRedisCredentialsProvider
A basic usage would look like;
mvn formatter:format
target. Don’t submit any formatting related changes.